Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some cleanup for extended diagnostic branch #1

Open
wants to merge 5 commits into
base: diagnostics
Choose a base branch
from

Conversation

karzhenkov
Copy link
Owner

No description provided.

@karzhenkov karzhenkov mentioned this pull request Jan 24, 2021
5 tasks
@nlohmann
Copy link

nlohmann commented Jan 24, 2021

I see what you are doing, and it seems to me the code could be simplified as follows:

  • Instead of passing a diagnostics_t object, we pass a basic_json::reference to the exceptions (just as you did).
  • But instead of using templates, that parameter could just default to nullptr; that is, a null JSON value.
  • This value will never occur in the parent relation, so the diagnostic string will always be empty.

This will have the same code as you have on the call site, but may have a simplified diagnostics_t class. What do you think?

@karzhenkov
Copy link
Owner Author

basic_json is a template, so a function taking basic_json::reference cannot be non-templated; there is no universal null JSON value. I think, the further simplification is quite possible, but in a different way. The diagnostics_t class can be eliminated; its primary functionality can be moved to diagnostics_message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants